[refactor] Centralize filesystem entry classification in inventory flow #276
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the inventory-building flow to centralize filesystem entry type detection behind IFileSystemInspector.ClassifyEntry(FileSystemInfo), consolidating symlink and POSIX-type handling so InventoryBuilder no longer performs scattered checks.
Changes:
- Added
ClassifyEntry(FileSystemInfo)toIFileSystemInspectorand implemented it inFileSystemInspector(symlink → POSIX → dir/file fallback → unknown). - Refactored
InventoryBuilderto useFileSystemInspector.ClassifyEntry(...)for skip decisions (symlink + POSIX special files). - Updated/added unit tests: refactored
InventoryBuilderInspectorTestsmocks and introducedFileSystemInspectorTests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ByteSync.Client.UnitTests/Services/Inventories/InventoryBuilderInspectorTests.cs | Updates mocks to support the new ClassifyEntry contract and adjusts skip-path tests accordingly. |
| tests/ByteSync.Client.UnitTests/Services/Inventories/FileSystemInspectorTests.cs | Adds coverage for the new centralized classification logic (directory/file/symlink/POSIX special/null). |
| src/ByteSync.Client/Services/Inventories/InventoryBuilder.cs | Replaces direct POSIX + reparse-point checks with centralized ClassifyEntry calls and removes now-redundant helper methods. |
| src/ByteSync.Client/Services/Inventories/FileSystemInspector.cs | Implements ClassifyEntry and introduces internal POSIX classifier wiring plus link-target/reparse-point helpers. |
| src/ByteSync.Client/Interfaces/Controls/Inventories/IFileSystemInspector.cs | Extends inspector interface with ClassifyEntry. |
Comments suppressed due to low confidence (1)
src/ByteSync.Client/Services/Inventories/InventoryBuilder.cs:406
- Symlink/reparse-point skips no longer emit any log message (previously
IsReparsePoint(...)logged warnings). This reduces observability when inventories unexpectedly skip content. Consider adding a warning/info log whenClassifyEntry(...)returnsSymlink(either here or inside the classifier), consistent with other skip reasons.
var entryKind = FileSystemInspector.ClassifyEntry(fileInfo);
if (entryKind == FileSystemEntryKind.Symlink)
{
RecordSkippedEntry(inventoryPart, fileInfo, SkipReason.Symlink, FileSystemEntryKind.Symlink);
return true;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| catch (Exception) | ||
| { | ||
| return FileSystemEntryKind.Unknown; |
There was a problem hiding this comment.
In ClassifyEntry, the POSIX classification catch returns Unknown immediately. That short-circuits the intended fallback to Directory/RegularFile classification and can change behavior if the POSIX classifier throws. Consider swallowing/logging the exception and continuing to the type-based fallback instead of returning early.
| return FileSystemEntryKind.Unknown; | |
| // Ignore POSIX classification failures and fall back to type-based classification below. |
|
|
||
| public interface IFileSystemInspector | ||
| { | ||
| FileSystemEntryKind ClassifyEntry(FileSystemInfo fsi); |
There was a problem hiding this comment.
ClassifyEntry defensively handles null inputs, but the interface contract uses a non-nullable FileSystemInfo parameter. With nullable enabled, consider changing the signature to FileSystemInfo? to reflect the supported input and avoid callers needing null! in tests.
| FileSystemEntryKind ClassifyEntry(FileSystemInfo fsi); | |
| FileSystemEntryKind ClassifyEntry(FileSystemInfo? fsi); |
1 participant
Summary:
Key changes:
Notes/risks: